Skip to content

Add state management, undo / redo capabilities and component API - #324

Open
handreyrc wants to merge 9 commits into
open-workflow-specification:mainfrom
handreyrc:add-undo-redo
Open

Add state management, undo / redo capabilities and component API #324
handreyrc wants to merge 9 commits into
open-workflow-specification:mainfrom
handreyrc:add-undo-redo

Conversation

@handreyrc

Copy link
Copy Markdown
Contributor

Closes #318

Summary

This PR adds state management as the model changes, undo/redo capabilities by handling a stack of states, and implements an API to expose those features.

Changes

  • Added state management integrated to the store, driven by model changes.
  • Added a stack of states where new states are pushed on top.
  • Added undo/redo capabilities and an API to trigger them and cause the diagram to load the stored states.
  • Added means to store and restore viewport state (zoom and pan) along with the model state.
  • Added means to restore selection if the name of the task has not changed.
  • Added an API to expose getContent, setContent, undo, redo, canUndo, canRedo, and colorMode, so it is possible to interact with the editor component by exposing the editor's ref and calling those functions from the browser console, making it easier to integrate with external components.
  • DiagramEditor, store, diagram error handling, and I18n were refactored and optimized to accommodate state management and the changes in the contextProvider.
  • Added the fast-equals library to detect structural changes between the current model and the new model.
    • Added a custom comparison function to ignore class-based internals, handle circular references, and treat field order as insignificant, so objects with the same fields and values in any order are considered equal.
  • Added an Undo/Redo story under features with a Docs section detailing the features and a story where all implemented features can be tested.

Copilot AI lite review requested due to automatic review settings August 11, 2026 20:51
@handreyrc handreyrc self-assigned this Aug 11, 2026
@netlify

netlify Bot commented Aug 11, 2026

Copy link
Copy Markdown

Deploy Preview for openworkflow-editor ready!

Name Link
🔨 Latest commit a08782c
🔍 Latest deploy log https://app.netlify.com/projects/openworkflow-editor/deploys/6a7f22841418620008b175c0
😎 Deploy Preview https://deploy-preview-324--openworkflow-editor.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Adds editor state/history management to support undo/redo (including viewport + selection restore) and exposes a new imperative API on the editor ref for integration/testing.

Changes:

  • Introduces generic useHistory and workflow-specific useWorkflowHistory hooks, plus structural equality comparison to avoid redundant history entries.
  • Refactors Diagram and store context provider to seed/history-track models and restore viewport/selection during undo/redo.
  • Adds Storybook feature story + tests for history behavior and ref API; adds fast-equals dependency.

Reviewed changes

Copilot reviewed 18 out of 20 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
pnpm-workspace.yaml Adds fast-equals to the workspace catalog.
packages/open-workflow-diagram-editor/package.json Adds fast-equals dependency for structural comparisons.
packages/open-workflow-diagram-editor/src/core/hooks/structuralEqual.ts Implements constructor-agnostic deep structural equality with circular handling.
packages/open-workflow-diagram-editor/src/react-flow/hooks/useHistory.ts Adds generic past/present/future history reducer + hook with stack cap.
packages/open-workflow-diagram-editor/src/react-flow/hooks/useWorkflowHistory.ts Adds workflow-aware history snapshots (model/viewport/selection) + undo/redo behavior.
packages/open-workflow-diagram-editor/src/store/DiagramEditorContext.tsx Extends context type with history + content-format APIs.
packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx Seeds history from content, exposes imperative API, and wires history into context.
packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx Gates ReactFlow mount until first layout, submits snapshots, and restores viewport on undo/redo.
packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx Refactors editor shell + docs and attempts to expose imperative API via context provider.
packages/open-workflow-diagram-editor/src/styles.css Removes stray trailing whitespace.
packages/open-workflow-diagram-editor/stories/features/UndoRedoEditor.tsx Adds a Storybook wrapper with undo/redo toolbar + window-exposed ref.
packages/open-workflow-diagram-editor/stories/features/UndoRedo.stories.tsx Adds Storybook docs + interactive story for undo/redo and ref API.
packages/open-workflow-diagram-editor/tests/react-flow/hooks/useHistory.test.ts Adds unit tests for generic history reducer/hook behavior.
packages/open-workflow-diagram-editor/tests/react-flow/hooks/useWorkflowHistory.test.ts Adds tests for workflow history snapshots, equality behavior, and viewport restore.
packages/open-workflow-diagram-editor/tests/core/hooks/structuralEqual.test.ts Adds comprehensive tests for structural equality across class/plain + circular refs.
packages/open-workflow-diagram-editor/tests/react-flow/diagram/Diagram.test.tsx Updates tests to wait for delayed ReactFlow mount after layout gating.
packages/open-workflow-diagram-editor/tests/store/DiagramEditorContextProvider.test.tsx Updates expected render cycles due to history seeding effect.
packages/open-workflow-diagram-editor/tests/diagram-editor/DiagramEditor.test.tsx Expands tests for ref API (undo/redo/getContent/setContent) and async canvas-dependent UI.
.changeset/state-management.md Publishes a minor version bump describing new state/history + API.
Files not reviewed (1)
  • pnpm-lock.yaml: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx Outdated
Comment thread packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx Outdated
Comment thread packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx Outdated
Comment thread packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx Outdated
Comment on lines +193 to +195
// setIsReadOnly is intentionally inoperative: isReadOnly is driven by
// props, not internal state, so there is no local setter to dispatch to.
setIsReadOnly: () => {},

@handreyrc handreyrc Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed! I opted for removing setIsReadOnly from DiagramEditorContextType.
@lornakelly @fantonangeli @kumaradityaraj, lets be careful with this one. I couldn't find any side effect but it is good to double check it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@handreyrc opening the preview and settings isReadOnly to false, I could not move the nodes in the diagram. Am I missing somenthing?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, maybe I missed this PR:
#302

Comment thread packages/open-workflow-diagram-editor/stories/features/UndoRedoEditor.tsx Outdated
Copilot AI review requested due to automatic review settings August 11, 2026 22:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 21 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (8)

packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:51

  • The comment says the content format is fixed at mount time, but setContent() can update contentFormat.current. This is misleading documentation and makes it harder to reason about getContent() behavior.
  // Detect the serialization format once from the initial content prop.
  // JSON content starts with `{` (after trimming); everything else is YAML.
  // We use a ref so the format is fixed at mount time and never flips mid-session
  // (an undo/redo should round-trip back in the same format the host provided).

packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:89

  • Undo/redo changes the model from history, but errors are currently tied to props.content. Deriving errors from the current model keeps validation/error-highlighting consistent across undo/redo snapshots, while still using parse errors when no model is available.
  // parseWorkflow drives both errors and the external-content model source.
  // errors are never part of a snapshot — always recomputed from current content.
  const { model: parsedModel, errors } = React.useMemo(
    () => parseWorkflow(props.content),
    [props.content],

packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:131

  • If applyAutoLayout throws on the initial render, layoutReady stays false and the ReactFlow canvas never mounts, leaving the editor blank. Consider falling back to rendering the un-laid-out graph (or at least setting layoutReady to true) on non-abort errors.
        .catch((error) => {
          if (error.name === "AbortError") {
            return;
          }
          console.error("Failed to apply auto-layout:", error);

packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx:86

  • This prop doc says the serialization format is preserved for the lifetime of the component, but the ref API docs (and tests) indicate the format can change after a successful setContent() call. Please align the documentation with the actual behavior.
   * The serialisation format is auto-detected on first load and preserved for
   * the lifetime of the component — see `getContent()` on `DiagramEditorRef`.

packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx:183

  • DiagramEditor computes a fallback locale (and uses it for <I18nProvider> and the lang attribute), but DiagramEditorBody passes the raw props.locale down into DiagramEditorContextProvider. If locale is omitted at runtime, the context provider can receive undefined and diverge from the I18n provider.
            props={props}

packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:19

  • Undo/redo changes the model from history, but errors are derived from parseWorkflow(props.content) and therefore won’t match the restored snapshot. This can make error highlighting inconsistent after undo/redo or after imperative setContent() (which doesn’t change props.content).

This issue also appears on line 85 of the same file.

import { buildFlatGraph, getTaskReferences, parseWorkflow } from "../core";

packages/open-workflow-diagram-editor/stories/features/UndoRedo.stories.tsx:101

  • The Storybook docs state that getContent() format is fixed at mount time, but the implementation/tests describe format switching after a successful setContent() call. This section is internally inconsistent (it later says the format becomes the new format) — please make the docs unambiguous.
Serialises the current model back to the **same format the editor received
on first load**: if the initial \`content\` prop was JSON it returns JSON; if
it was YAML it returns YAML. The format is fixed at mount time and preserved
for the lifetime of the component, so undo/redo always round-trips in the
original format. Returns \`""\` when no valid model has been loaded yet.

packages/open-workflow-diagram-editor/tests/test-utils/render-helpers.tsx:45

  • DiagramEditorContextType now requires contentFormat and the history API members, but the test mock context value doesn’t provide them. This should be a type error and may also cause runtime issues in tests that rely on these fields.
  edges: [],
  taskReferences: new Set(),
  selectedNodeId: null,
  setLocale: noop,
  setEdges: noop,
  setNodes: noop,

@lornakelly

Copy link
Copy Markdown
Collaborator

Thanks for PR @handreyrc, looks really good, have just tested the story so far but noticed a couple of things:

  • setContent doesnt seem to be parsing the model fully as its not validating, the validation gets triggered when you undo/redo
  • Also, we should hide the toolbar when isReadOnly is true as currently it allows you to edit when it is true
Screen.Recording.2026-08-12.at.10.46.38.mov

@fantonangeli fantonangeli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a small comment which you can consider

Comment on lines +25 to +32
export type HistoryState<T> = {
/** Past snapshots, oldest first. Length is capped at HISTORY_STACK_SIZE. */
past: T[];
/** The current snapshot. Null when the history has not been initialised yet. */
present: T | null;
/** Future snapshots available for redo. Index 0 is the most recently undone entry. */
future: T[];
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current history implementation uses three separate arrays (past, present, future) which works correctly. However, I wanted to share an alternative pattern that might simplify the code:

type HistoryState<T> = {
  history: T[];
  presentIndex: number;
};

This way future is simply presentIndex+1.
Wdyt?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fantonangeli,

Sure, if we can make it simpler why not?!

I changed the implementation following your recommendation.

Thanks!

Comment on lines +193 to +195
// setIsReadOnly is intentionally inoperative: isReadOnly is driven by
// props, not internal state, so there is no local setter to dispatch to.
setIsReadOnly: () => {},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, maybe I missed this PR:
#302

Copilot AI review requested due to automatic review settings August 12, 2026 13:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 20 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (3)

packages/open-workflow-diagram-editor/src/core/hooks/structuralEqual.ts:81

  • innerEquals doesn’t short-circuit when comparing the same reference (e.g. a === b). In this PR the history pipeline calls structuralEqual(present.model, model) frequently with identical object references, so missing this fast-path can turn routine viewport/selection updates into expensive deep traversals.
): boolean {
  if (isObjectLike(a) && isObjectLike(b)) {

packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:221

  • Viewport pan/zoom changes don’t appear to be persisted into the current history snapshot: submitModel(...) is only called after layout cycles (and indirectly on selection changes via the effect deps), but there is no subscription to viewport changes. That means if a user pans/zooms and then later triggers an undo/redo, the restored viewport can be stale (typically the last fitView/restored value, not where the user was looking). Hook into React Flow viewport updates (e.g. a viewport/move end callback or store subscription) and call submitModel(model, viewport, selectedNodeId) so useWorkflowHistory can update the present snapshot without pushing a new entry.
          onNodesChange={onNodesChange}
          onEdgesChange={onEdgesChange}
          onSelectionChange={onSelectionChange}
          onlyRenderVisibleElements={true}
          zoomOnDoubleClick={false}
          elementsSelectable={true}
          panOnScroll={true}
          panOnDrag={false}
          zoomOnScroll={false}
          preventScrolling={true}
          selectionOnDrag={true}
          fitView
          fitViewOptions={{ ...FIT_VIEW_OPTIONS, duration: 0 }}

packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:51

  • The comment says contentFormat is fixed at mount time and “never flips mid-session”, but setContent() later updates contentFormat.current (and contentFormatVersion exists specifically to re-render when it changes). This is misleading documentation and makes it harder to reason about the ref API contract.
  // Detect the serialization format once from the initial content prop.
  // JSON content starts with `{` (after trimming); everything else is YAML.
  // We use a ref so the format is fixed at mount time and never flips mid-session
  // (an undo/redo should round-trip back in the same format the host provided).

Comment thread packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx Outdated
Copilot AI review requested due to automatic review settings August 12, 2026 14:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 20 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (1)

packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx:119

  • DiagramEditorContent renders ParsingErrorPage whenever model is null. Since DiagramEditorContextProvider seeds history in a useEffect, valid content briefly produces model=null on the initial render, causing an incorrect error page flicker. Gate the error page on the presence of actual parse errors (or render a neutral placeholder) until the initial parse/seed completes.
  const { model } = useDiagramEditorContext();
  return model === null ? (
    <ParsingErrorPage />
  ) : (
    <Diagram divRef={diagramDivRef} colorMode={colorMode} />
  );

@handreyrc

Copy link
Copy Markdown
Contributor Author

Thanks for PR @handreyrc, looks really good, have just tested the story so far but noticed a couple of things:

  • setContent doesnt seem to be parsing the model fully as its not validating, the validation gets triggered when you undo/redo
  • Also, we should hide the toolbar when isReadOnly is true as currently it allows you to edit when it is true

Screen.Recording.2026-08-12.at.10.46.38.mov

@lornakelly,

The toolbar does not make sense in all contexts the component can be used, however, we need it to showcase how to consume the API so it was completely moved to the "Undo Redo" story and is not part of the editor component anymore.
The validation issue with the setContent should be fixed.

Thanks for reviewing this PR!

Copilot AI review requested due to automatic review settings August 12, 2026 14:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 21 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (2)

packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:121

  • Selection preservation during layout rebuild only stamps selected: true onto the selected node. If the selected element is an edge, the rebuilt edge list will not mark it selected, so React Flow will drop the edge selection and z-index won’t reflect the selection.
            const stampedNodes = selectedId
              ? nodes.map((n) => (n.id === selectedId ? { ...n, selected: true } : n))
              : nodes;
            setNodes(stampedNodes);
            setEdges(applyEdgeZIndex(edges));

packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:99

  • onSelectionChange only considers selected nodes and ignores selected edges, so selecting an edge clears selectedNodeId. This prevents edge selection from being preserved across content reloads and undo/redo snapshots (which expect node/edge IDs).

This issue also appears on line 117 of the same file.

  const onSelectionChange = React.useCallback<RF.OnSelectionChangeFunc>(
    ({ nodes: selectedNodes }) => setSelectedNodeId(selectedNodes[0]?.id ?? null),
    [setSelectedNodeId],
  );

@handreyrc
handreyrc requested a review from fantonangeli August 12, 2026 15:06
@handreyrc

Copy link
Copy Markdown
Contributor Author

@fantonangeli @lornakelly @kumaradityaraj ,

This PR is ready for reviewing again.

Thanks

@fantonangeli fantonangeli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks a lot @handreyrc

Comment thread packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx Outdated
Comment thread packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx Outdated
Comment thread packages/open-workflow-diagram-editor/src/core/structuralEqual.ts
@fantonangeli

Copy link
Copy Markdown
Member

@handreyrc I found a bug with the zoom:

Testing this with the last PR merged on main, the zoom doesn't get a reset: https://deploy-preview-319--openworkflow-editor.netlify.app/?path=/story/use-cases-workflows--multi-agent-ai-content-generation

Screencast.From.2026-08-13.12-35-13.mp4

Copilot AI review requested due to automatic review settings August 13, 2026 16:17
Signed-off-by: handreyrc <handrey.cunha@gmail.com>
Signed-off-by: handreyrc <handrey.cunha@gmail.com>
Signed-off-by: handreyrc <handrey.cunha@gmail.com>
Signed-off-by: handreyrc <handrey.cunha@gmail.com>
Signed-off-by: handreyrc <handrey.cunha@gmail.com>
Signed-off-by: handreyrc <handrey.cunha@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 22 out of 24 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (12)

packages/open-workflow-diagram-editor/stories/features/UndoRedo.stories.tsx:1

  • The story docs contradict themselves: one section states the format is fixed at mount time, while another states setContent() updates the format for future getContent() calls. Align the docs with the implemented behavior (and keep it consistent with the DiagramEditorRef docstring).
/*

packages/open-workflow-diagram-editor/stories/features/UndoRedo.stories.tsx:1

  • The story docs contradict themselves: one section states the format is fixed at mount time, while another states setContent() updates the format for future getContent() calls. Align the docs with the implemented behavior (and keep it consistent with the DiagramEditorRef docstring).
/*

packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:70

  • layoutError is latched permanently: once set, the component returns <ErrorPage /> and never attempts layout again, even if subsequent model changes would succeed. Reset layoutError to null at the start of the layout effect (or when model/errors change) so recovery is possible without a full remount.
  const [layoutError, setLayoutError] = React.useState<Error | null>(null);

packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:188

  • layoutError is latched permanently: once set, the component returns <ErrorPage /> and never attempts layout again, even if subsequent model changes would succeed. Reset layoutError to null at the start of the layout effect (or when model/errors change) so recovery is possible without a full remount.
        .catch((error) => {
          if (error.name === "AbortError") {
            return;
          }
          setLayoutError(error instanceof Error ? error : new Error(String(error)));
        });

packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:140

  • selectedNodeId can represent either a node or an edge (e.g. resolveSelectedId checks both), but the selection preservation here only stamps selected: true on nodes. If an edge is selected, it will be lost when edges are replaced, and applyEdgeZIndex will never see edge.selected. Preserve selection for edges too (e.g., stamp selected: true on the matching edge before applying zIndex).
            // Preserve selection: stamp selected:true on the node that matches
            // selectedNodeId so React Flow does not clear it when nodes are replaced.
            const selectedId = selectedNodeIdRef.current;
            const stampedNodes = selectedId
              ? nodes.map((n) => (n.id === selectedId ? { ...n, selected: true } : n))
              : nodes;
            setNodes(stampedNodes);
            setEdges(applyEdgeZIndex(edges));

packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx:150

  • The component computes a resolved locale (with detectLocale(...) fallback) and uses it for the root lang, but passes props.locale into DiagramEditorContextProvider. If a JS consumer omits the prop (or passes undefined), the provider can receive an invalid locale while the root uses the fallback. Pass the resolved locale variable into the provider to keep behavior consistent.
        <DiagramEditorContextProvider
          ref={editorRef}
          content={props.content}
          isReadOnly={props.isReadOnly}
          locale={props.locale}
        >

packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx:169

  • The component computes a resolved locale (with detectLocale(...) fallback) and uses it for the root lang, but passes props.locale into DiagramEditorContextProvider. If a JS consumer omits the prop (or passes undefined), the provider can receive an invalid locale while the root uses the fallback. Pass the resolved locale variable into the provider to keep behavior consistent.
    const locale = React.useMemo(() => {
      const supportedLocales = Object.keys(dictionaries);
      return props.locale ?? detectLocale(supportedLocales);
    }, [props.locale]);

packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:52

  • The comment says the content format is fixed at mount time and never flips, but setContent explicitly updates contentFormat.current. Please update the comment (and any related docs) to reflect the actual behavior: either (a) format is fixed for the session, or (b) format tracks the most recently successfully loaded content.
  // Detect the serialization format once from the initial content prop.
  // JSON content starts with `{` (after trimming); everything else is YAML.
  // We use a ref so the format is fixed at mount time and never flips mid-session
  // (an undo/redo should round-trip back in the same format the host provided).
  const contentFormat = React.useRef<ContentFormat>(
    props.content.trimStart().startsWith("{") ? "json" : "yaml",
  );

packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:146

  • The comment says the content format is fixed at mount time and never flips, but setContent explicitly updates contentFormat.current. Please update the comment (and any related docs) to reflect the actual behavior: either (a) format is fixed for the session, or (b) format tracks the most recently successfully loaded content.
      const newFormat: ContentFormat = content.trimStart().startsWith("{") ? "json" : "yaml";
      if (newFormat !== contentFormat.current) {
        contentFormat.current = newFormat;
        setContentFormatVersion((v) => v + 1);
      }

packages/open-workflow-diagram-editor/stories/features/UndoRedo.stories.tsx:191

  • navigator.clipboard.writeText(...) can reject (permissions/HTTP context), which will currently create an unhandled promise rejection in Storybook. Add a .catch(...) handler (and ideally surface a message/state) to keep the story stable.
  const copyToClipboard = () => {
    navigator.clipboard.writeText(getContentText).then(() => {
      setCopied(true);
      setTimeout(() => setCopied(false), 2000);
    });
  };

packages/open-workflow-diagram-editor/tests/store/DiagramEditorContextProvider.test.tsx:82

  • Asserting exact render counts is brittle across React versions, StrictMode settings, and internal refactors (especially with effects and concurrent rendering). Prefer asserting observable behavior (e.g., context values, errors, history state) rather than the number of renders.
    // Two rendering cycles are expected:
    // 1- initial render, 2- useEffect seeding history from parsedModel
    expect(renderCount).toHaveTextContent(/2/i);

packages/open-workflow-diagram-editor/tests/react-flow/diagram/Diagram.test.tsx:350

  • This timing-based wait is likely to be flaky in CI and slows the suite. Prefer fake timers (vi.useFakeTimers() + advancing timers) or waiting on a deterministic condition via waitFor rather than sleeping for a fixed duration.
      await act(async () => {
        await new Promise((resolve) => setTimeout(resolve, 50));
      });

Copilot AI review requested due to automatic review settings August 13, 2026 16:36
Signed-off-by: handreyrc <handrey.cunha@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 22 out of 24 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (7)

packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:55

  • contentFormat is updated in setContent(...), but not when props.content changes. This makes getContent() potentially serialize in the wrong format after an external content prop update (and contradicts the DiagramEditorRef doc/comment that format tracks the latest successfully loaded content). Update contentFormat inside the props.content effect as well (only when parsing succeeded) so the format stays consistent regardless of whether content changes come from props or the imperative API.
  const [contentFormat, setContentFormat] = React.useState<ContentFormat>(
    props.content.trimStart().startsWith("{") ? "json" : "yaml",
  );

packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:102

  • contentFormat is updated in setContent(...), but not when props.content changes. This makes getContent() potentially serialize in the wrong format after an external content prop update (and contradicts the DiagramEditorRef doc/comment that format tracks the latest successfully loaded content). Update contentFormat inside the props.content effect as well (only when parsing succeeded) so the format stays consistent regardless of whether content changes come from props or the imperative API.
  React.useEffect(() => {
    const { model: parsedModel, errors: parsedErrors } = parseWorkflow(props.content);
    setErrors(parsedErrors);
    if (parsedModel === null) {
      // Null model is never stored in history.
      return;
    }

packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:140

  • Selection preservation is only applied to nodes. If selectedNodeId points to an edge (your provider explicitly allows this via resolveSelectedId), React Flow can still clear edge selection when edges are replaced, and applyEdgeZIndex won’t elevate the selected edge because edge.selected is never stamped. Consider stamping selected: true on the matching edge as well (and then applying zIndex) so edge selections survive re-layout consistently.
            // Preserve selection: stamp selected:true on the node that matches
            // selectedNodeId so React Flow does not clear it when nodes are replaced.
            const selectedId = selectedNodeIdRef.current;
            const stampedNodes = selectedId
              ? nodes.map((n) => (n.id === selectedId ? { ...n, selected: true } : n))
              : nodes;
            setNodes(stampedNodes);
            setEdges(applyEdgeZIndex(edges));

packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx:150

  • DiagramEditor computes a resolved locale (with detectLocale) for <I18nProvider> and the root lang attribute, but passes the raw props.locale into DiagramEditorContextProvider. This can desync useI18n() (resolved locale) from useDiagramEditorContext().locale (raw locale). Pass the resolved locale variable into the context provider to keep the UI language and context locale consistent.
        <DiagramEditorContextProvider
          ref={editorRef}
          content={props.content}
          isReadOnly={props.isReadOnly}
          locale={props.locale}
        >

packages/open-workflow-diagram-editor/stories/features/UndoRedo.stories.tsx:269

  • The modals are missing dialog semantics: the container uses role="none" and there’s no role="dialog", aria-modal="true", or labeling via aria-labelledby. Even for Storybook-only UI, adding proper dialog roles/labels improves keyboard + screen-reader behavior and avoids regressions if this pattern is copied into product code.
      {getContentOpen && (
        <div style={overlayStyle} onMouseDown={() => setGetContentOpen(false)} role="presentation">
          <div style={dialogStyle} onMouseDown={(e) => e.stopPropagation()} role="none">
            <div style={dialogHeaderStyle}>Get Content</div>
            <textarea
              style={{ ...textareaStyle, cursor: "default", userSelect: "text" }}
              value={getContentText}
              readOnly
              spellCheck={false}
            />

packages/open-workflow-diagram-editor/tests/store/DiagramEditorContextProvider.test.tsx:82

  • These assertions depend on an exact render count, which is an implementation detail and tends to become brittle across React/testing-library upgrades and minor refactors (especially around effects). Prefer asserting on observable state/output changes (e.g., that model/errors are correct after seeding) rather than the number of render cycles.
    // Two rendering cycles are expected:
    // 1- initial render, 2- useEffect seeding history from parsedModel
    expect(renderCount).toHaveTextContent(/2/i);

packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:154

  • The post-layout setTimeout(..., 0) isn’t cleared in the effect cleanup. While the isActive guard prevents state updates, the queued task can still run after unmount and call into reactFlowInstance. Track the timeout id and clear it in the cleanup to avoid stray calls and make the lifecycle more robust under rapid content changes/unmounts.
            // Post-layout viewport work runs in a zero-delay timeout so React Flow has
            // processed the new nodes before we read or set the viewport.
            setTimeout(() => {
              if (!isActive) return;

              const pendingRestore = pendingViewportRestoreRef.current;
              if (pendingRestore) {
                // Undo/redo — restore saved viewport instead of fitting.
                reactFlowInstance.setViewport(pendingRestore);
                clearPendingViewportRestoreRef.current();

Copilot AI review requested due to automatic review settings August 13, 2026 16:55
Signed-off-by: handreyrc <handrey.cunha@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 22 out of 24 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (7)

packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:137

  • Selection stamping during layout only marks nodes as selected. If the current selection is an edge ID, React Flow will drop the edge selection when edges are replaced, and applyEdgeZIndex() will never treat the selected edge as selected. Stamp the selected edge too before applying z-index.
            // Preserve selection: stamp selected:true on the node that matches
            // selectedNodeId so React Flow does not clear it when nodes are replaced.
            const selectedId = selectedNodeIdRef.current;
            const stampedNodes = selectedId
              ? nodes.map((n) => (n.id === selectedId ? { ...n, selected: true } : n))

packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:102

  • When props.content becomes unparseable after a valid model was loaded, this effect returns early and leaves the previous model in history as the rendered model. That means the editor can show a diagram that no longer matches the content prop, and the ParsingErrorPage will never appear for subsequent parse failures. Consider resetting history/present to null (or providing a reset action in useWorkflowHistory/useHistory) when parsedModel is null.
  React.useEffect(() => {
    const { model: parsedModel, errors: parsedErrors } = parseWorkflow(props.content);
    setErrors(parsedErrors);
    if (parsedModel === null) {
      // Content is unparseable — reset history to null so downstream consumers
      // (e.g. DiagramEditorContent) see model === null and render the error page

packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:115

  • onSelectionChange only considers selected nodes, so selecting an edge will clear selection (selectedNodeId becomes null) and edge selection cannot be preserved/restored (undo/redo, content reload). Include selected edges when computing the selected ID.

This issue also appears on line 133 of the same file.

  const onSelectionChange = React.useCallback<RF.OnSelectionChangeFunc>(
    ({ nodes: selectedNodes }) => setSelectedNodeId(selectedNodes[0]?.id ?? null),
    [setSelectedNodeId],
  );

packages/open-workflow-diagram-editor/stories/features/UndoRedo.stories.tsx:588

  • The story docs claim getContent()'s format is fixed at mount time, but later in the same docs setContent() is described as switching the format (and the implementation/tests also switch formats). Update this section to avoid contradicting the behavior.
Serialises the current model back to the **same format the editor received
on first load**: if the initial \`content\` prop was JSON it returns JSON; if
it was YAML it returns YAML. The format is fixed at mount time and preserved
for the lifetime of the component, so undo/redo always round-trips in the
original format. Returns \`""\` when no valid model has been loaded yet.

packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:124

  • Once layoutError is set, the component permanently renders the ErrorPage even if a later model/errors change would allow layout to succeed, because layoutError is never cleared. Clear layoutError when starting a new layout attempt (or on success).
  React.useEffect(() => {
    let isActive = true;
    let abortController: AbortController | null = null;

packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:111

  • contentFormat is initialised from the initial content prop and updated by setContent(), but it is not updated when the external content prop changes. If the host swaps YAML↔JSON via props, getContent() will serialize using a stale format.
  // In read-only mode the placeholder viewport is acceptable since fitView always runs.
  React.useEffect(() => {
    const { model: parsedModel, errors: parsedErrors } = parseWorkflow(props.content);
    setErrors(parsedErrors);
    if (parsedModel === null) {
      // Content is unparseable — reset history to null so downstream consumers
      // (e.g. DiagramEditorContent) see model === null and render the error page

packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx:86

  • The content prop doc says the serialization format is preserved for the lifetime of the component, but the implementation explicitly allows the format to change after a successful setContent() call. Align this comment with the actual contract (format follows the latest successfully loaded content and is preserved across undo/redo).
   * Updating this prop (e.g. from an addon panel) re-parses the workflow and,
   * in edit mode, pushes a new history entry if the model changed structurally.
   * The serialisation format is auto-detected on first load and preserved for
   * the lifetime of the component — see `getContent()` on `DiagramEditorRef`.

Copilot AI review requested due to automatic review settings August 13, 2026 17:15
@handreyrc

Copy link
Copy Markdown
Contributor Author

@handreyrc I found a bug with the zoom:

Testing this with the last PR merged on main, the zoom doesn't get a reset: https://deploy-preview-319--openworkflow-editor.netlify.app/?path=/story/use-cases-workflows--multi-agent-ai-content-generation

Screencast.From.2026-08-13.12-35-13.mp4

@fantonangeli,

Good catch!

It is fixed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 22 out of 24 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (6)

packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:137

  • Selection preservation during layout only stamps selected: true onto nodes. If the current selection is an edge (or if edge selection is later supported), replacing the edges array after auto-layout can clear the selection in React Flow. Stamp the selection onto edges as well before setting them into state.
            // Preserve selection: stamp selected:true on the node that matches
            // selectedNodeId so React Flow does not clear it when nodes are replaced.
            const selectedId = selectedNodeIdRef.current;
            const stampedNodes = selectedId
              ? nodes.map((n) => (n.id === selectedId ? { ...n, selected: true } : n))

packages/open-workflow-diagram-editor/stories/features/UndoRedo.stories.tsx:588

  • The docs for getContent() are internally inconsistent: they say the serialization format is fixed at mount time, but the setContent() section below says the format is auto-detected from the supplied string and becomes the new format. This should match the actual API behavior (format follows the most recently successfully loaded content).
Serialises the current model back to the **same format the editor received
on first load**: if the initial \`content\` prop was JSON it returns JSON; if
it was YAML it returns YAML. The format is fixed at mount time and preserved
for the lifetime of the component, so undo/redo always round-trips in the
original format. Returns \`""\` when no valid model has been loaded yet.

packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:115

  • onSelectionChange only tracks selected nodes and ignores selected edges, but the rest of the codebase (history snapshot + docs) treats selectedNodeId as a node-or-edge selection. This causes edge selection to be dropped and prevents undo/redo from restoring edge selection.

This issue also appears on line 133 of the same file.

  const onSelectionChange = React.useCallback<RF.OnSelectionChangeFunc>(
    ({ nodes: selectedNodes }) => setSelectedNodeId(selectedNodes[0]?.id ?? null),
    [setSelectedNodeId],
  );

packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:124

  • Once layoutError is set, it is never cleared, so the component will keep rendering the auto-layout error page even if subsequent model/content updates would succeed. Clearing the error at the start of the layout effect allows recovery when content changes.
  React.useEffect(() => {
    let isActive = true;
    let abortController: AbortController | null = null;

packages/open-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx:185

  • DiagramEditor computes a normalized/fallback locale for the lang attribute and I18nProvider, but it passes the raw props.locale into DiagramEditorContextProvider. This can lead to the context/store using a different locale than the i18n provider (and breaks the fallback when props.locale is missing/unsupported at runtime). Pass the normalized locale through instead.
          <DiagramEditorBody
            diagramDivRef={diagramDivRef}
            resolvedColorMode={resolvedColorMode}
            props={props}
            editorRef={ref}

packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:100

  • When the external content prop changes between YAML and JSON, contentFormat is not updated (it’s only updated by setContent). This can make getContent() serialize in the wrong format after a host-driven content reload, which conflicts with the contract that format follows the last successfully loaded content and is preserved across undo/redo.
  React.useEffect(() => {
    const { model: parsedModel, errors: parsedErrors } = parseWorkflow(props.content);
    setErrors(parsedErrors);
    if (parsedModel === null) {

@handreyrc

handreyrc commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

@lornakelly @fantonangeli @kumaradityaraj,

This PR is ready for reviewing again.

Thanks

@fantonangeli

fantonangeli commented Aug 14, 2026

Copy link
Copy Markdown
Member

@handreyrc, before I re-review this, it seems the test logs are getting really big:
in this PR, which was before my fix on test messages, there where 3000 lines:
https://github.com/open-workflow-specification/editor/actions/runs/31373703826/job/93408133949
but in your PR are 17580, can you please check why there are so many messages?

Also, I think it would be good if you sync with main because my PR to fix many test logs has been merged.

@lornakelly lornakelly left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending fabrizios feedback

Signed-off-by: handreyrc <handrey.cunha@gmail.com>
Copilot AI review requested due to automatic review settings August 14, 2026 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: Implement state management, undo / redo capabilities and component API

5 participants